feat(opentelemetry-collector): add VPA and in-place resize support#2076
feat(opentelemetry-collector): add VPA and in-place resize support#2076boqu wants to merge 9 commits intoopen-telemetry:mainfrom
Conversation
boqu
requested review from
a team,
TylerHelmuth,
dmitryax and
povilasv
as code owners
|
@TylerHelmuth Thank you for your review. I pushed all the changes according to your review. Please let me know. |
|
The CI failed due missing VPA CRD. See https://github.com/open-telemetry/opentelemetry-helm-charts/actions/runs/22117983560/job/63930896483?pr=2076. VPA CRDs are not part of core Kubernetes. Is it ok to add VPA CRDs in the CI test cluster? |
|
I would be strongly against bringing non-standard crds to this chart. Is there currently a way to install VPA separately? |
|
@boqu does |
I added a minimal stub VPA CRDs here. But it's failing with below error. If my understanding is correct, helm fails when resolving all resource types against the cluster API servers before applying anything. So it looks like we still need to install the CRDs before the CI test. |
I think we only need to install the CRDs in the github action to run the CI test. If this is not acceptable, I can simply remove CI test. |
|
I install the VPA CRD in the github setup action. The CI test can pass now. |
|
|
||
| # Vertical Pod Autoscaler - https://kubernetes.io/docs/concepts/workloads/autoscaling/#vertical-pod-autoscaling | ||
| verticalPodAutoscaler: | ||
| enabled: false |
There was a problem hiding this comment.
I have misunderstood how this feature is implemented in kubernetes. Is this feature not available in a standard +1.27 k8s install? What are the additional CRDs it needs?
There was a problem hiding this comment.
No, the VPA doesn't come with kubernetes in default. It need to be installed separately. See https://kubernetes.io/docs/concepts/workloads/autoscaling/#scaling-workloads-vertically and https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler. The CRD can be found here.
There was a problem hiding this comment.
We've typically been against adding direct support for external CRDs, but seeing as how this is a kubernetes-owned CRD I'm ok adding this feature. @open-telemetry/helm-approvers what do you think?
There was a problem hiding this comment.
Can I know if we can move this PR forward or anything outstanding concern?
There was a problem hiding this comment.
I'd appreciate if we can move this PR forward.
There was a problem hiding this comment.
@TylerHelmuth Anything I can do to move this PR forward?
There was a problem hiding this comment.
I would be ok with having native VPA support.
But didnt understand why extraManifess didnt work?
This error seems that the VPA CRDs wasnt installed on that cluster:
Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest: resource mapping not found for name: "opentelemetry-collector-o6n8pl90u0" namespace: "opentelemetry-collector-o6n8pl90u0" from "": no matches for kind "VerticalPodAutoscaler" in version "autoscaling.k8s.io/v1"
There was a problem hiding this comment.
My understanding is, before applying anything, the helm renders the whole release and runs every manifest through the client‑side REST mapper to resolve its GVK(Group, Version and Kind) against the API server's discovery. At that moment, the CRD added via extraManifest has not been posted. Thus, helm fails to build VPA object. That also explains why it works if we pre-install VPA CRD in the CI test. See .github/actions/setup/action.yaml.
There was a problem hiding this comment.
Ya you can't install a CRD via the manifest. Managing CRDs in the helm chart that also installs something that relies on those CRDs is really hard (see the kube-stack chart). If you install the CRDs separately I'd expect the extraManifest to work. We will definitely not be adding the CRDs to this chart, so no matter what you'll need to manage the CRDs elsewhere.
There was a problem hiding this comment.
I guess I probably didn't describe the PR clearly in the first place. The OTEL collector helm chart won't install VPA CRDs. If users use VPA with the helm chart, they need to make sure the VPA CRDs have been installed already. I just updated the comment to make it clear.
I also bump the version to resolve the merge conflicts.
Can we consider to move this PR forward?
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
boqu
force-pushed
the
add-support-for-vpa
branch
from
e094768 to
a5e0b92
Compare
|
Any chance we can move this PR forward? |
Bump chart to 0.153.0 to resolve version conflict with upstream (0.152.0) and re-render examples. Strengthen VPA comment in values.yaml to note that VPA CRDs and controller must be installed separately — this chart does not install them.
4 participants
Summary
resizePolicysupport for Kubernetes in-place pod resource resize (>= 1.27)VPA
templates/vpa.yamltemplate gated onautoscaling.k8s.io/v1API capability andverticalPodAutoscaler.enabledrecommenders,controlledResources,maxAllowed,minAllowed,updatePolicy, and fullcontainerPoliciesoverridevpaKindhelper in_helpers.tplvalues.schema.jsonci/vpa-deployment-values.yamlIn-place resize
resizePolicyfield on the collector container in_pod.tplNotRequiredandRestartContainerrestart policies per resource